Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate no space between expression and ? in ternaries #22523

Merged
merged 2 commits into from
Jun 29, 2017

Conversation

ararslan
Copy link
Member

@ararslan ararslan commented Jun 25, 2017

Discussing with @JeffBezanson and @StefanKarpinski, 0.7 will have this deprecation warning, then in 1.0 x? will mean something different (likely Union{T,Null}) rather than an error.

@ararslan ararslan added deprecation This change introduces or involves a deprecation parser Language parsing and surface syntax labels Jun 25, 2017
@ararslan ararslan requested a review from JeffBezanson June 25, 2017 01:42
@JeffBezanson
Copy link
Member

+100 to adding the spaces to the code in Base. The deprecation might need more discussion.

@iamed2
Copy link
Contributor

iamed2 commented Jun 26, 2017

Regarding the deprecation:

It might be odd for people coming directly from 0.6 to 1.0 getting this:

julia> x? 1:3
ERROR: syntax: extra token "1" after end of expression

Perhaps we could have a two-stage deprecation where this error prints some extra information like "using ternary syntax without whitespace was deprecated in 0.7"?

@ararslan
Copy link
Member Author

I think the goal with 1.0 is to have no deprecations at all. Typically we do two-stage deprecations, but in this case I think it's something we'd like to get into 1.0, so in order to make that happen we need to bypass our standard deprecation practice a bit. It's unfortunate but worth it IMO.

@tkelman
Copy link
Contributor

tkelman commented Jun 26, 2017

Syntax that is an error in 1.0 could probably be given a non-error meaning in 1.x. A syntax error is a questionable thing to depend on.

@ararslan
Copy link
Member Author

ararslan commented Jun 26, 2017

If we want to have a good story for missing data in 1.0, being able to write Int? to mean Union{Int, Null} (or whatever) would be much nicer than always having to write out the full type. It'd be a little unfortunate to have to wait for 1.x (for x > 0) for a better syntax for something we want in 1.0.

@iamed2
Copy link
Contributor

iamed2 commented Jun 26, 2017

By two-stage deprecation I meant:

  1. Deprecation warning
  2. New behaviour, but in cases which would work with old behaviour but error with new behaviour, generate helpful error messages

@ararslan
Copy link
Member Author

Ah, so in 1.0 make sure that x? 1:3 emits a message more helpful than "extra token after end of expression"? That sounds good to me. 👍

base/printf.jl Outdated
'+' in flags ? push!(blk.args, :(write(out, neg?'-':'+'))) :
' ' in flags ? push!(blk.args, :(write(out, neg?'-':' '))) :
'+' in flags ? push!(blk.args, :(write(out, neg ? '-':'+'))) :
' ' in flags ? push!(blk.args, :(write(out, neg ? '-':' '))) :
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps add spaces around the colons as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I missed that. Yes, good idea, thanks.

@ararslan ararslan force-pushed the aa/dep-tern-no-space branch from d0fb3a2 to 03d1187 Compare June 26, 2017 21:10
@ararslan
Copy link
Member Author

@JeffBezanson Do you have reservations about the deprecation? What kind of discussion needs to take place here?

Copy link
Member

@Sacha0 Sacha0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm (modulo the parser changes, which I have not pored over )! :)

(Discussion elsewhere suggests consensus formed in the affirmative re. the deprecation since the initial comments on this PR.)

@tkelman
Copy link
Contributor

tkelman commented Jun 29, 2017

We shouldn't plan on introducing any new features between 0.7 and 1.0, to avoid making that take any longer than just removing deprecations.

@ararslan
Copy link
Member Author

I feel like this is something we should really have in 1.0. I know the timing skirts around our usual things a bit, but I really think it's worth it in this case. Having a good story for missing data and a sensible way to express it is pretty crucial in a scientific language and thus far we've been lacking in that area. Being able to write T? = Union{T, Base.Null} rather than writing out the Union will make 1.0 that much more appealing for statisticians, data scientists, etc.

@ararslan
Copy link
Member Author

But at any rate, we can cross that bridge when we come to it. Since just the deprecation here seems pretty uncontroversial, I'll go ahead and merge this.

@ararslan ararslan merged commit e91c0ff into master Jun 29, 2017
@ararslan ararslan deleted the aa/dep-tern-no-space branch June 29, 2017 18:23
@tkelman tkelman added the needs news A NEWS entry is required for this change label Jun 29, 2017
@tkelman
Copy link
Contributor

tkelman commented Jun 29, 2017

That definition doesn't have to be in base, if the operator is officially undefined for 1.0. Adding features between 0.7 and 1.0 is making that into a separate release cycle which would go against the publicly announced plan.

I think syntax sugar is far from the data ecosystem's biggest problem that needs to be solved before 1.0.

@ararslan
Copy link
Member Author

I think syntax sugar is far from the data ecosystem's biggest problem that needs to be solved before 1.0.

Certainly. The rest is being actively worked on and at this point I think we have a fairly solid plan.

@StefanKarpinski
Copy link
Member

Not allowing ourselves to claim back syntaxes that are deprecated in 0.7 is like trying to work with both arms tied behind our backs. I think we should plan to reclaim some syntaxes, but we need to have PRs for them ready to go when we tag 0.7, then merge them and tag 1.0. Actually, they should all be ready on a single branch.

@tkelman
Copy link
Contributor

tkelman commented Jun 29, 2017

So much for 1.0 == 0.7 without deprecations then, it's actually going to be 0.7 without deprecations plus some to-be-determined amount of new stuff? Everything that gets added requires testing (in a few trial use cases, not only within base), documenting, stabilization before it's release-worthy. aka a separate release cycle which we've said there wouldn't be between 0.7 and 1.0.

@StefanKarpinski
Copy link
Member

Please don't be so dramatic, the key is that the releases come out back-to-back and have minimal differences. Even making T? not a parse error and an operator as you yourself suggested above requires parser changes between 0.7 and 1.0. A few parser changes that we've staged and tested beforehand doesn't make it a full release cycle. Chill.

@JeffBezanson
Copy link
Member

To me the important thing is that code with no warnings on 0.7 also works on 1.0. If you fix the warnings, your code will not contain X?, so adding a meaning for X? is pretty unlikely to break anything. But if we need more time for nullable stuff to settle we could also add it in 1.1.

@tkelman
Copy link
Contributor

tkelman commented Jun 30, 2017

If 0.7 is in feature freeze for a while, a lot of things will be pending and "ready." That feature freeze should extend across both 0.7 and 1.0 or the schedule is going to slip further.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecation This change introduces or involves a deprecation parser Language parsing and surface syntax
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants